Bluetooth: Host: Add role-specific auto PHY update options #97198

jhedberg · 2025-10-08T13:15:00Z

The BT_AUTO_PHY_UPDATE option was problematic in the sense that it didn't
really allow any kind of fine-tuning of what automation is desired. In
particular, it didn't allow specifying which PHY was the preferred one (it
was hardcoded to 2M) and also didn't allow specifying role-specific
(central vs peripheral) preferences.

To solve this, deprecate the old option, and instead introduce
role-specific options that allow specifying 1M/2M/Coded/None as the
preference. The new options have slightly different defaults than the new:
for central role 2M stays as the preference, however since it's uncommon to
require automated changes on the peripheral side the default is set to
None there.

subsys/bluetooth/host/Kconfig

doc/releases/migration-guide-4.3.rst

subsys/bluetooth/host/conn.c

subsys/bluetooth/host/Kconfig

jhedberg · 2025-10-09T09:47:28Z

Posting here the latest from Discord:

There shouldn't be a need for any of this automation in the host, neither for the PHY nor for the data length, due to the existence of HCI_LE_Set_Default_PHY and HCI_LE_Write_Suggested_Default_Data_Length commands that could either be included as part of the host's HCI init (taking their values from Kconfig), or from the application, for which we already have existing APIs.

Actually, we already have HCI_LE_Write_Suggested_Default_Data_Length in the HCI init, so the code in conn.c is (as the code comment there indicates) only needed for buggy controllers that don't otherwise take care of this. Either way, the Kconfig options for these become questionable.

The only place where the CONFIG_BT_USER_DATA_LEN_UPDATE and CONFIG_BT_USER_PHY_UPDATE options are slightly useful are for memory saving in the bt_conn_cb struct. However, they're really not related to triggers from the app side, rather a peripheral app may be interested in these e.g. if the central goes and changes the values during a connection.

jhedberg · 2025-10-09T11:32:56Z

Conclusion from the Bluetooth WG meeting:

The set_default HCI commands don't actually cause any procedures to be triggered in themselves, so the explicit triggering by the host still has a purpose. I'll therefore roll back to my earlier plan to just do the role split for the phy update. For data length, we can either remove or deprecate the "auto" Kconfig option, since it serves no purpose now that the controller "no_auto_dle" quirk comes from DTS, i.e. is build-time resolvable in itself.

Another improvement that can be done (follow-up PR) is to cache the controller max data length, since we read it already during HCI init, i.e. no need to redo this for every connection.

cvinayak · 2025-10-09T11:45:00Z

Just re-iterating my statements from the WG meeting.

just do the role split for the phy update.

Agree. And keep the feature available (default disabled is ok) for users needing auto PHY update being handled by Host on connection establishment (i.e. while connected callback is generated to applications). We do not want such users investing in developing a solution themself when migrating to new Zephyr release.

For data length, we can either remove or deprecate the "auto" Kconfig option, since it serves no purpose now that the controller "no_auto_dle" quirk comes from DTS, i.e. is build-time resolvable in itself.

Agree.

Another improvement that can be done (follow-up PR) is to cache the controller max data length, since we read it already during HCI init, i.e. no need to redo this for every connection.

Agree.

alwa-nordic · 2025-10-09T11:48:46Z

Another improvement that can be done (follow-up PR) is to cache the controller max data length, since we read it already during HCI init, i.e. no need to redo this for every connection.

HCI spec does not prohibit asking for a larger value than the Controller supports. We can always do bt_le_set_data_len(conn, 0x00fb, 0x4290), which are the HCI-defined absolute limits.

Thalley · 2025-10-09T11:57:08Z

HCI spec does not prohibit asking for a larger value than the Controller supports. We can always do bt_le_set_data_len(conn, 0x00fb, 0x4290), which are the HCI-defined absolute limits.

From 7.8.33 LE Set Data Length command (similar text exists for 7.8.35 LE Write Suggested Default Data Length command):

This command allows the Host to suggest the maximum transmission payload size and
maximum packet transmission time (connMaxTxOctets and connMaxTxTime - see [Vol
6] Part B, Section 4.5.10) to be used for LL Data PDUs on a given connection. The
Controller may use smaller or larger values based on local information.

I think that make might sense to do. We probably want to use the largest data length, and since the host isn't using the results of bt_hci_le_read_max_data_len for anything anyways, then it's kind of moot to perform that (not to mention that even setting the data length to the maximum value supported by the controller, doesn't ensure that it is indeed the value that the controller supports.
Of course as @jhedberg mentioned, some controllers may have some odd behavior (e.g. could just ignore the value from the host if it's outside it's range, instead of applying the maximum it supports).

jhedberg · 2025-10-10T08:54:00Z

@Thalley @cvinayak I've pushed an update to implement the initial change as discussed

Thalley · 2025-10-10T09:12:12Z

subsys/bluetooth/host/conn.c

+	switch (conn->role) {
+#if defined(AUTO_PHY_CENTRAL)
+	case BT_HCI_ROLE_CENTRAL:
+		if (AUTO_PHY_CENTRAL_SUPPORTED(bt_dev.le.features) &&
+		    phy_change_needed(conn, AUTO_PHY_CENTRAL)) {
+			err = bt_le_set_phy(conn, 0U, AUTO_PHY_CENTRAL_PREF,
+					    AUTO_PHY_CENTRAL_PREF, BT_HCI_LE_PHY_CODED_ANY);
 		}
-		if (conn->state != BT_CONN_CONNECTED) {
-			return;
+		break;
+#endif
+#if defined(AUTO_PHY_PERIPHERAL)
+	case BT_HCI_ROLE_PERIPHERAL:
+		if (AUTO_PHY_PERIPHERAL_SUPPORTED(bt_dev.le.features) &&
+		    phy_change_needed(conn, AUTO_PHY_PERIPHERAL)) {
+			err = bt_le_set_phy(conn, 0U, AUTO_PHY_PERIPHERAL_PREF,
+					    AUTO_PHY_PERIPHERAL_PREF, BT_HCI_LE_PHY_CODED_ANY);
 		}
+		break;
+#endif
+	default:
+		/* No PHY preferences set for the given role */
+		break;
+	}


Given that phy_change_needed takes conn, the conn->role check could be moved to that function which would make the code here significantly simpler

I'm all for finding ways to simplify this. However, I'm not sure how that exact proposal helps, since we also need the role to know what to pass to the bt_le_set_phy() call.

since we also need the role to know what to pass to the bt_le_set_phy() call.

Since the value is exclusively based on the conn->role, then we don't need to pass it along at all. The function can just take the conn pointer and nothing more :)

Thalley · 2025-10-10T09:13:38Z

subsys/bluetooth/host/conn.c

 */
-static bool uses_symmetric_2mbit_phy(struct bt_conn *conn)
+#if defined(AUTO_PHY_CENTRAL) || defined(AUTO_PHY_PERIPHERAL)
+static bool phy_change_needed(struct bt_conn *conn, uint8_t phy)


Minor: "Needed" is a bit strongly worded here; we never really need to update the PHY, do we?

(Not a blocker, and the name is OK, but otherwise consider can_update_phy or something instead?)

I'm not sure "can" is any more intuitive here. This is whether we already have the PHY we prefer. If we do, then we don't "need" to do the procedure. What other word would be better?

If we end up moving everything related to the PHY update into the function (see #97198 (comment)), it would probably just be do_auto_phy_update :D

Yes, I can certainly refactor this so that everything is in a single PHY-related function. It might have a slight impact on code size, but likely not much (if any)

Thalley · 2025-10-10T09:15:24Z

subsys/bluetooth/host/Kconfig

+config BT_AUTO_PHY_PERIPHERAL_NONE
+	bool "No PHY preference"


I would consider putting "None" as the first choice

Btw, the _NONE options shouldn't really be needed since Kconfig has the optional keyword for choice statements which make it possible to not have any of the possible options selected. The problem with optional (as I discovered) is that it then doesn't allow us to have a default where one of the options would anyway be selected. The only effect of a default for optional is that when you select the choice on the top-level in menuconfig, then the default is what'll be initially enabled, however the top-level doesn't implicitly get enabled by specifying a default.

There's a regression from commit 8cfad44 which introduced trying to enable advertising twice. Remove the other attempt. Signed-off-by: Johan Hedberg <[email protected]>

The BT_AUTO_PHY_UPDATE option was problematic in the sense that it didn't really allow any kind of fine-tuning of what automation is desired. In particular, it didn't allow specifying which PHY was the preferred one (it was hardcoded to 2M) and also didn't allow specifying role-specific (central vs peripheral) preferences. To solve this, deprecate the old option, and instead introduce role-specific options that allow specifying 1M/2M/Coded/None as the preference. The new options have slightly different defaults than the new: for central role 2M stays as the preference, however since it's uncommon to require automated changes on the peripheral side the default is set to None there. Signed-off-by: Johan Hedberg <[email protected]>

Document the changes related to Bluetooth auto PHY update Kconfig options. Signed-off-by: Johan Hedberg <[email protected]>

Remove deprecated Kconfig option usage, and replace it with corresponding options which yield the same behavior as before. Signed-off-by: Johan Hedberg <[email protected]>

Make sure to consider both the new peripheral and central Kconfig options when calculating needed TX buffers. Signed-off-by: Johan Hedberg <[email protected]>

jhedberg · 2025-10-13T12:56:51Z

@Thalley @cvinayak There was a test case bug that @aescolar helped to uncover. See the first commit in this PR for the details. Fingers crossed that everything will be green now.

aescolar · 2025-10-13T13:19:48Z

We can probably also find a way to convert the values to strings using strerror in some way

I'd say that would be nicer. Otherwise the next thing would be someone looking at one libc errno.h or another and getting confused. Just faster to see the error name printed.

jhedberg · 2025-10-13T13:35:13Z

@Thalley @cvinayak There was a test case bug that @aescolar helped to uncover. See the first commit in this PR for the details. Fingers crossed that everything will be green now.

Turns out this was a red herring :( It was a bug, but not the cause of the test case failure. Only other idea I have is to 100% restore the PHY config for this test case, in case it depends on both sides to have "auto-phy 2M" enabled.

jhedberg · 2025-10-13T14:02:33Z

Now we have

d_00: @00:00:07.159852 ERROR: (WEST_TOPDIR/zephyr/tests/bsim/bluetooth/audio/src/bap_scan_delegator_test.c:141): PA timeout

Thalley · 2025-10-13T14:10:17Z

Now we have

d_00: @00:00:07.159852 ERROR: (WEST_TOPDIR/zephyr/tests/bsim/bluetooth/audio/src/bap_scan_delegator_test.c:141): PA timeout

I also get that for #91587.
When the timing changes, the scheduling changes, and sometimes that causes issues like this.

Sometimes it helps nudging the -start_offset and -rs values in the test script, but it's pretty hard to know what values to set (I managed to get the above to pass locally, but fail in CI). Maybe @cvinayak can help provide some reasonable values there?

aescolar · 2025-10-13T16:47:16Z

(I managed to get the above to pass locally, but fail in CI)

@Thalley that should not be happening unless there is unitialized memory in play (or the test is killed in CI due to taking too long).

Thalley · 2025-10-13T17:25:12Z

(I managed to get the above to pass locally, but fail in CI)

@Thalley that should not be happening unless there is unitialized memory in play (or the test is killed in CI due to taking too long).

I've seen that happen on many occasions over the years, and I've never been able to find any memory issues with valgrind, ASAN or UBSAN :)

jhedberg · 2025-10-14T07:09:29Z

@Thalley why hasn't the test been at least disabled, if a proper fix is not known? Blocking other PRs is justification enough to treat fixing or disabling it as a hotfix.

I can disable as part of this PR, or would you like to do it separately?

cvinayak · 2025-10-14T07:58:19Z

@Thalley why hasn't the test been at least disabled, if a proper fix is not known? Blocking other PRs is justification enough to treat fixing or disabling it as a hotfix.

I can disable as part of this PR, or would you like to do it separately?

This PR changes the default timeline of many tests, causing scheduling overlaps or change in connection event length. Restoring back the use of PHY update for the failing tests should make it pass, have you tried that?

jhedberg · 2025-10-14T08:08:52Z

This PR changes the default timeline of many tests, causing scheduling overlaps or change in connection event length. Restoring back the use of PHY update for the failing tests should make it pass, have you tried that?

I haven't, since @Thalley pointed out that the same test is also failing for #91587 (which doesn't change any timings, AFAIK)

Thalley · 2025-10-14T08:15:48Z

@Thalley why hasn't the test been at least disabled, if a proper fix is not known? Blocking other PRs is justification enough to treat fixing or disabling it as a hotfix.

I can disable as part of this PR, or would you like to do it separately?

Because it's not specific to this test. This behavior/issue can somewhat randomly happen to any test (and especially any LE Audio test, given that we are often doing quite a lot on air in those tests with multiple devices).

I haven't, since @Thalley pointed out that the same test is also failing for #91587 (which doesn't change any timings, AFAIK)

The PR here does change the timing, since it removes a few steps in a function.

The real problem is, IMO, the scheduling overlaps that isn't being handled well. In a real-world device, it would/should probably do a retry if it fails to sync to the PA or any similar issue (often triggered by a user interaction), but that isn't the case with the tests today.

Restore the auto-phy config for tests which seem particularly sensitive to timings related to this. Signed-off-by: Johan Hedberg <[email protected]>

jhedberg · 2025-10-14T08:59:40Z

Just pushed an update not to modify the auto-phy stuff for the audio bsim tests.

sonarqubecloud · 2025-10-14T09:22:42Z

Quality Gate passed

Issues
4 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

jhedberg · 2025-10-14T10:21:08Z

@Thalley @cvinayak @aescolar CI is all green now. Thanks for your help with debugging this!

cvinayak · 2025-10-14T11:16:08Z

tests/bsim/bluetooth/ll/conn/src/test_connect2.c

-	err = bt_le_adv_start(BT_LE_ADV_CONN_FAST_1, ad, ARRAY_SIZE(ad), NULL, 0);
-	if (err) {
-		FAIL("Advertising failed to start (err %d)\n", err);
-		return;
-	}
-
 	err = start_advertising();
-


Similar change but retains FAIL in the test in this PR: #97479

The test code seems to use printk() and FAIL() kind of interchangeably. E.g. don't you get duplicated output with your PR since start_advertising() has an internal printk() for the failure case?

zephyrbot added area: Bluetooth Release Notes To be mentioned in the release notes area: Bluetooth Host Bluetooth Host (excluding BR/EDR) labels Oct 8, 2025

zephyrbot requested review from HaavardRei, PavelVPV, Thalley, alwa-nordic, cfriedt, cvinayak, hermabe, kartben, rugeGerritsen and sjanc October 8, 2025 13:16

zephyrbot assigned jhedberg and alwa-nordic Oct 8, 2025

jhedberg requested a review from nordicjm October 8, 2025 13:21

Thalley reviewed Oct 8, 2025

View reviewed changes

subsys/bluetooth/host/Kconfig Outdated Show resolved Hide resolved

subsys/bluetooth/host/Kconfig Show resolved Hide resolved

jhedberg force-pushed the periph_phy_update branch from c33aaac to b93d436 Compare October 8, 2025 13:52

Thalley reviewed Oct 8, 2025

View reviewed changes

doc/releases/migration-guide-4.3.rst Outdated Show resolved Hide resolved

subsys/bluetooth/host/conn.c Outdated Show resolved Hide resolved

subsys/bluetooth/host/conn.c Outdated Show resolved Hide resolved

subsys/bluetooth/host/Kconfig Show resolved Hide resolved

jhedberg added the Bluetooth Review Discussion in the Bluetooth WG meeting required label Oct 9, 2025

jhedberg force-pushed the periph_phy_update branch from b93d436 to d6b78bb Compare October 10, 2025 08:52

jhedberg changed the title ~~Bluetooth: Host: Limit auto-phy update to central by default~~ Bluetooth: Host: Add role-specific auto PHY update options Oct 10, 2025

Thalley reviewed Oct 10, 2025

View reviewed changes

jhedberg force-pushed the periph_phy_update branch from d6b78bb to cc810c8 Compare October 10, 2025 09:47

jhedberg added 5 commits October 13, 2025 15:55

Bluetooth: tests: bsim: Fix double advertising in test_connect2

0e0f425

There's a regression from commit 8cfad44 which introduced trying to enable advertising twice. Remove the other attempt. Signed-off-by: Johan Hedberg <[email protected]>

doc: release-notes-4.3: Document Bluetooth auto PHY update changes

6bd4b21

Document the changes related to Bluetooth auto PHY update Kconfig options. Signed-off-by: Johan Hedberg <[email protected]>

Bluetooth: samples/tests: Update auto PHY usage to recent changes

38fb28e

Remove deprecated Kconfig option usage, and replace it with corresponding options which yield the same behavior as before. Signed-off-by: Johan Hedberg <[email protected]>

Bluetooth: Update TX buffer calculation with auto PHY changes

dff2e4c

Make sure to consider both the new peripheral and central Kconfig options when calculating needed TX buffers. Signed-off-by: Johan Hedberg <[email protected]>

jhedberg force-pushed the periph_phy_update branch from 6b578db to dff2e4c Compare October 13, 2025 12:56

Thalley previously approved these changes Oct 13, 2025

View reviewed changes

jhedberg dismissed Thalley’s stale review via 646596c October 13, 2025 13:37

jhedberg closed this Oct 13, 2025

github-project-automation bot moved this from In Review to Done in Bluetooth LE Audio Oct 13, 2025

jhedberg reopened this Oct 13, 2025

Bluetooth: tests: bsim: Restore auto PHY config

f170248

Restore the auto-phy config for tests which seem particularly sensitive to timings related to this. Signed-off-by: Johan Hedberg <[email protected]>

jhedberg force-pushed the periph_phy_update branch from 646596c to f170248 Compare October 14, 2025 08:55

cvinayak approved these changes Oct 14, 2025

View reviewed changes

alwa-nordic approved these changes Oct 14, 2025

View reviewed changes

github-project-automation bot moved this from Done to In Review in Bluetooth LE Audio Oct 14, 2025

Bluetooth: Host: Add role-specific auto PHY update options #97198

Are you sure you want to change the base?

Bluetooth: Host: Add role-specific auto PHY update options #97198

Conversation

jhedberg commented Oct 8, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jhedberg commented Oct 9, 2025

Uh oh!

jhedberg commented Oct 9, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cvinayak commented Oct 9, 2025

Uh oh!

alwa-nordic commented Oct 9, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Thalley commented Oct 9, 2025

Uh oh!

jhedberg commented Oct 10, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jhedberg commented Oct 13, 2025

Uh oh!

aescolar commented Oct 13, 2025

Uh oh!

jhedberg commented Oct 13, 2025

Uh oh!

jhedberg commented Oct 13, 2025

Uh oh!

Thalley commented Oct 13, 2025

Uh oh!

aescolar commented Oct 13, 2025

Uh oh!

Thalley commented Oct 13, 2025

Uh oh!

jhedberg commented Oct 14, 2025

Uh oh!

cvinayak commented Oct 14, 2025

Uh oh!

jhedberg commented Oct 14, 2025

Uh oh!

Thalley commented Oct 14, 2025

Uh oh!

jhedberg commented Oct 14, 2025

Uh oh!

sonarqubecloud bot commented Oct 14, 2025

Quality Gate passed

Uh oh!

jhedberg commented Oct 14, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jhedberg Oct 14, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

jhedberg commented Oct 8, 2025 •

edited

Loading

jhedberg commented Oct 9, 2025 •

edited

Loading

alwa-nordic commented Oct 9, 2025 •

edited

Loading

jhedberg Oct 14, 2025 •

edited

Loading